Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Gracefully handle primitive extensions in a backwards-compatible way #1

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

viggyfresh
Copy link

Derived from the issue thread here: smart-on-fhir#30

Specifically implements the "underscored shadow fields" suggested here: smart-on-fhir#30 (comment) with the implementation strategy espoused here: smart-on-fhir#30 (comment)

Consistent in implementation style to https://github.com/smart-on-fhir/Swift-FHIR, just takes the alternate approach to custom FHIR* fields for primitives since we already heavily rely on the primitives directly (would be nasty to switch every usage of fhir_resource.string to fhir_resource.fhir_string.value).

@viggyfresh viggyfresh changed the title wip: will this work??? feat: Gracefully handle primitive extensions in a backwards-compatible way Sep 10, 2024
@JiahanD
Copy link

JiahanD commented Sep 10, 2024

Have we taken a look at the serialization/deserialization layer? It makes more sense to put the functionality to handle primitive extensions there rather than adding primitive extensions to every primitive field

https://github.com/smart-on-fhir/client-py/blob/63b408168aa556c825689987d326db36163bbdbb/fhirclient/models/fhirabstractbase.py#L207C12-L207C65

@viggyfresh
Copy link
Author

Have we taken a look at the serialization/deserialization layer? It makes more sense to put the functionality to handle primitive extensions there rather than adding primitive extensions to every primitive field

https://github.com/smart-on-fhir/client-py/blob/63b408168aa556c825689987d326db36163bbdbb/fhirclient/models/fhirabstractbase.py#L207C12-L207C65

hmm as far as I can tell that code successfully stores primitives on the python object but doesn't serialize them (which is the as_json method below it and the source of our problems). the way to register them for serialization in fhirclient is actually the approach taken in this PR (which adds them to elementProperties). we could technically omit the changes to the instance variables on the resource class itself, but I figure it is useful for clarity.

@viggyfresh
Copy link
Author

Have we taken a look at the serialization/deserialization layer? It makes more sense to put the functionality to handle primitive extensions there rather than adding primitive extensions to every primitive field
https://github.com/smart-on-fhir/client-py/blob/63b408168aa556c825689987d326db36163bbdbb/fhirclient/models/fhirabstractbase.py#L207C12-L207C65

hmm as far as I can tell that code successfully stores primitives on the python object but doesn't serialize them (which is the as_json method below it and the source of our problems). the way to register them for serialization in fhirclient is actually the approach taken in this PR (which adds them to elementProperties). we could technically omit the changes to the instance variables on the resource class itself, but I figure it is useful for clarity.

i guess another option is to just override the serializer there, but that would involve even more hardcoding and checks I think (and break out of the validation we get for free here - if you try to pass an incompatible primitive extension it actually yells at you)

@JiahanD
Copy link

JiahanD commented Sep 10, 2024

So I think why this seems convoluted to me is this repo doesn't actually model out all the different primitive datatypes in fhir, they've began it with fhirdate, and some of the other date/time related datatypes but they don't have all of them like string, int, markdown, oid, uuid, canonical, etc.

I think to get the serializer/deserializer to work we'd need to have a primitive abstract base class that inherits from an element abstract base class, and the primitive data classes will inherit the primitive ABC and then we can have changes to the serializer/deserializer to parse the fields correctly.

@viggyfresh
Copy link
Author

So I think why this seems convoluted to me is this repo doesn't actually model out all the different primitive datatypes in fhir, they've began it with fhirdate, and some of the other date/time related datatypes but they don't have all of them like string, int, markdown, oid, uuid, canonical, etc.

I think to get the serializer/deserializer to work we'd need to have a primitive abstract base class that inherits from an element abstract base class, and the primitive data classes will inherit the primitive ABC and then we can have changes to the serializer/deserializer to parse the fields correctly.

yep - if you check out the issue discussion linked in the top, they actually suggest this as an approach.

HOWEVER, the blast radius of such a change is basically a full rewrite of all of our codebase that touches fhir. we (like every other fhirclient python consumer) have gotten used to doing fhir_resource.string_property and having that be a string, NOT fhir_resource.string_property.string_value or whatever. i rejected that sweeping change as a viable option, now and into the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants